mppx validate: multi-challenge support, per-method validation, streaming output#629
Conversation
09fafb7 to
5a9072e
Compare
commit: |
7bec8d5 to
9241fef
Compare
9241fef to
91e84bf
Compare
ea9256f to
ecc5e92
Compare
brendanjryan
left a comment
There was a problem hiding this comment.
@bensandler-stripe -- some CI issues but content-wise this look good!
0eca153 to
e474a0c
Compare
|
ah @brendanjryan sorry if I wasted your time, I was adding a few other fixes to this branch. Will get your review in a few. |
b77cf89 to
fb93ddf
Compare
65bd823 to
6f5a7dc
Compare
|
@brendanjryan let's merge #631 first (this PR is now rebased off that branch) and then I'll have you review this one |
6f5a7dc to
a23b9ca
Compare
| @@ -0,0 +1,5 @@ | |||
| --- | |||
…ment loop - Multi-challenge parsing (fromResponseList) with per-method [tag] prefixes - Per-method field validation for Stripe, EVM, and Tempo - Payment loop tries each supported method with per-method prompts - Common field checks reduced across all challenges - URL resolution fix for subpath-mounted servers - BigInt guards with isValidIntegerAmount - Malformed challenge detection and reporting GIT_VALID_PII_OVERRIDE Committed-By-Agent: claude
a23b9ca to
09b0065
Compare
|
code-wise this LGTM
Why do we need checks like this? Shouldn't all of this be implicitly guarded by the There is some concern with servers passing invalid json on the wire, but IMO we should just fail are struct marshal time vs. trying to do field-level checks. This is much simpler to maintain |
@brendanjryan you mean, servers' usage of mppx (or another SDK) should prevent this from even happening? My thinking is, if we rejected at struct-marshal time, we'd just get "parse failed" with no actionable feedback. There was a real user's server I validated (this diff shows before/after) where we previously failed on an opaque Is your contention that we can't reasonably do this for every field, so perhaps it's not really worth doing for any? I think amounts specifically are reasonably tricky to get right, so I personally feel good about these validations. But I agree I wouldn't want to do the same for every other field. |
|
Yes -- main concern being that this is a little noisy and redundant, but I do see the benefit. I'm +1 to this given that all of this is isolated in a designated "validate" package |
brendanjryan
left a comment
There was a problem hiding this comment.
@bensandler-stripe -- some CI issues but content-wise this look good!
| ;(bad.request as Record<string, unknown>).amount = '1.50' | ||
| const server = await multiChallengeServer([bad]) | ||
| const { output } = await serve(['validate', server.url]) | ||
| expect(output).toContain('Amount is valid integer string (Got: 1.50)') |
There was a problem hiding this comment.
why is this "is a valid integer string?" shouldn't this be invalid?

A number of improvements to
mppx validate. Builds on #631.Bug fixes:
mppx validate https://api.example.com/mppnow correctly resolves to/mpp/openapi.jsoninstead of/openapi.json)isValidIntegerAmountregex check first (prevents crash on non-integer amounts like "5.000000")fromResponseListto parse all payment methods fromWWW-Authenticate(previously only picked the first one)UX and validation improvements:
recipientorsplitsDiffs before/after